Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include markers issue #5092 #5114

Merged
merged 9 commits into from
Jun 25, 2022
Merged

Include markers issue #5092 #5114

merged 9 commits into from
Jun 25, 2022

Conversation

ImreC
Copy link
Contributor

@ImreC ImreC commented May 30, 2022

@matteius @oz123 I think this is literally what needs to happen for this, after the changes that were done a month ago. Or am I overlooking something?

@ImreC ImreC changed the title Include markers #5092 Include markers issue #5092 May 30, 2022
@oz123
Copy link
Contributor

oz123 commented May 30, 2022

@ImreC thanks for contributing again.
I would appreciate if it, if you could include a test case and the usual news fragment.

Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment about test and news.

@ImreC
Copy link
Contributor Author

ImreC commented May 31, 2022

Yeah sure. Was just checking if this was literally it. Will do that later today

@matteius
Copy link
Member

matteius commented Jun 2, 2022

Yeah that seems like the logical change. @ImreC Not sure you timeline, but I'd like to do another pipenv release due to another issue in the next week or so and would love if we could include this one as well.

@ImreC
Copy link
Contributor Author

ImreC commented Jun 7, 2022

Hi @matteius, this completely fell off the map on my end. I'm having a very busy couple of weeks. I will find some time on Friday to do this

@ImreC
Copy link
Contributor Author

ImreC commented Jun 14, 2022

@oz123 @matteius I have now implemented it as an optional flag. Do you agree with this approach? Happy to make it default if that's better

@matteius
Copy link
Member

@ImreC I am thinking it should be the default behavior to include markers, but perhaps a flag like you have to exclude them (non-default) makes sense? Especially with reports like this, having the ability to exclude markers may be relevant for some:

Also, Not sure if you saw my comment the other day I CC'd you on another issue report, the TLDR is (having trouble finding the ticket link)

I do wonder if in the code I linked to for the requirements command, if we should have the logic first add the dev dependencies so that the production dependencies can overwrite any of those? -- that seems more correct to me than allowing the dev dependencies to break the production specifiers.

@oz123
Copy link
Contributor

oz123 commented Jun 18, 2022

I tend to agree here, the flag should actually be --exclude-markers and the default behaviour should include them.

@ImreC
Copy link
Contributor Author

ImreC commented Jun 24, 2022

@oz123 @matteius I flipped the markers flag. I really have been short for time the last couple of weeks. Hope this covers all.

@@ -281,6 +281,7 @@ flag::
pytest==3.2.3

Adding the ``--hash`` flag will add package hashes to the output for extra security.
Adding the ``--exclude-markers`` flagwill exclude the markers from the output.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo need space flag will -- I tried to edit this but I don't think I can directly without some hoop jumping.

if dev or dev_only:
deps.update(lockfile["develop"])
if not dev_only:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flipping the ordering here!

Copy link
Member

@matteius matteius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve @ImreC nice work -- just if you can please add the space for the typo so we can merge soon.

@oz123
Copy link
Contributor

oz123 commented Jun 24, 2022

@ImreC @matteius please also fix the merge conflict.

@matteius
Copy link
Member

@oz123 I am not seeing what you mean by a potential merge conflict ... If there is not one, I am ready to merge this and will push a commit to main to fix the documentation typo.

@oz123
Copy link
Contributor

oz123 commented Jun 25, 2022

Ay! Sorry depending on the type of merge, there will be a conflict or there won't be.
image

image

@oz123 oz123 merged commit 37b1fb4 into pypa:main Jun 25, 2022
@ImreC ImreC deleted the fix-missing-markers branch June 27, 2022 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants